-
-
Notifications
You must be signed in to change notification settings - Fork 18
Backend sign in audit #1453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backend sign in audit #1453
Conversation
- Added new entity `SignInAuditEntity` to track user sign-in attempts. - Created DTOs for creating and finding sign-in audit records. - Implemented repository interface and custom repository for sign-in audit operations. - Developed use cases for finding sign-in audit logs and creating sign-in audit records. - Integrated sign-in audit logging into existing user login use cases (usual and OTP login). - Added controller for handling sign-in audit log retrieval requests. - Updated user entity to establish a relationship with sign-in audit records. - Created migration to set up the sign-in audit table and its associated types. - Enhanced error handling and validation for sign-in methods and statuses.
… fields, and implement sign-in audit logging for Google and GitHub logins
…rId, add migration for sign-in audit table, and implement comprehensive end-to-end tests for sign-in audit logs
…uding filtering by status, email, sign-in method, and date range
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements comprehensive backend sign-in audit functionality to track and log all user authentication attempts across different sign-in methods (email, Google, GitHub, SAML, OTP).
Key changes:
- Created new
SignInAuditEntitywith database migration to store sign-in events including status, method, IP address, user agent, and failure reasons - Integrated audit logging into all authentication flows (email/password, OTP, Google OAuth, GitHub OAuth)
- Added API endpoint
/sign-in-audit/logswith filtering capabilities (by status, email, method, date range) and pagination - Included comprehensive test coverage for both SaaS and non-SaaS environments
- Fixed enum validation bug in
validate-string-with-enumhelper function
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/migrations/1764239409303-AddSignInAuditEntity.ts | Database migration creating signInAudit table with enum types for status and sign-in methods |
| backend/src/entities/user-sign-in-audit/sign-in-audit.entity.ts | Entity definition for sign-in audit records with relationships to user entity |
| backend/src/entities/user-sign-in-audit/sign-in-audit.service.ts | Service layer for creating sign-in audit records |
| backend/src/entities/user-sign-in-audit/sign-in-audit.controller.ts | REST API controller exposing GET endpoint for retrieving filtered audit logs |
| backend/src/entities/user-sign-in-audit/use-cases/find-sign-in-audit-logs.use.case.ts | Business logic for fetching and filtering sign-in audit logs with authorization checks |
| backend/src/entities/user-sign-in-audit/repository/sign-in-audit-custom-repository-extension.ts | Custom repository methods for querying audit logs with complex filters |
| backend/src/entities/user/use-cases/usual-login-use.case.ts | Integrated audit logging for email/password authentication at all failure and success points |
| backend/src/entities/user/use-cases/otp-login-use.case.ts | Added audit logging for OTP (two-factor) authentication flows |
| backend/src/microservices/saas-microservice/use-cases/login-with-google.use.case.ts | Integrated audit logging for Google OAuth authentication |
| backend/src/microservices/saas-microservice/use-cases/login-with-github.use.case.ts | Integrated audit logging for GitHub OAuth authentication |
| backend/src/entities/user/user.controller.ts | Extracts IP address and user agent from HTTP requests for audit logging |
| backend/src/helpers/validators/validate-string-with-enum.ts | Fixed validation to check enum values instead of keys |
| backend/src/exceptions/text/messages.ts | Added error messages for invalid sign-in status and method filters |
| backend/src/common/application/global-database-context.ts | Registered sign-in audit repository in global database context |
| backend/src/app.module.ts | Added SignInAuditModule to application imports |
| backend/test/ava-tests/saas-tests/user-sign-in-audit.e2e.test.ts | Comprehensive E2E tests covering all audit log scenarios for SaaS mode |
| backend/test/ava-tests/non-saas-tests/non-saas-user-sign-in-audit.e2e.test.ts | Comprehensive E2E tests for non-SaaS mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const userRepository = this.manager.getRepository(UserEntity); | ||
| const user = await userRepository.findOne({ where: { id: userId } }); | ||
| if (user) { | ||
| newRecord.user = user; |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistency: In the service (line 34), newRecord.userId = user.id is set, but in the repository extension (line 26), this assignment is missing. This could lead to the userId column not being properly populated when using the repository method directly. Add newRecord.userId = user.id; after line 26 in the repository extension to maintain consistency.
| newRecord.user = user; | |
| newRecord.user = user; | |
| newRecord.userId = user.id; |
| if (!user || user.company?.id !== companyId) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.COMPANY_NOT_FOUND, |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message Messages.COMPANY_NOT_FOUND is misleading in this context. This condition also covers the case where the user doesn't have permission to access the company's audit logs (user.company?.id !== companyId). A more appropriate message would be Messages.FORBIDDEN or a new message like Messages.USER_NOT_AUTHORIZED_FOR_COMPANY_AUDIT_LOGS.
| message: Messages.COMPANY_NOT_FOUND, | |
| message: Messages.FORBIDDEN, |
| import { TestUtils } from '../../utils/test.utils.js'; | ||
| import { Cacher } from '../../../src/helpers/cache/cacher.js'; | ||
| import { MockFactory } from '../../mock.factory.js'; | ||
| import { WinstonLogger } from '../../../src/entities/logging/winston-logger.js'; | ||
| import { SignInStatusEnum } from '../../../src/entities/user-sign-in-audit/enums/sign-in-status.enum.js'; | ||
| import { SignInMethodEnum } from '../../../src/entities/user-sign-in-audit/enums/sign-in-method.enum.js'; | ||
|
|
||
| let app: INestApplication; | ||
| let currentTest: string; | ||
| let testUtils: TestUtils; | ||
|
|
||
| const mockFactory = new MockFactory(); | ||
|
|
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variables mockFactory and testUtils are declared but never used in the test file. Consider removing them along with their imports (MockFactory and TestUtils) to keep the code clean. If they're intended for future use, add a comment explaining why they're kept.
| import { TestUtils } from '../../utils/test.utils.js'; | ||
| import { Cacher } from '../../../src/helpers/cache/cacher.js'; | ||
| import { MockFactory } from '../../mock.factory.js'; | ||
| import { WinstonLogger } from '../../../src/entities/logging/winston-logger.js'; | ||
| import { SignInStatusEnum } from '../../../src/entities/user-sign-in-audit/enums/sign-in-status.enum.js'; | ||
| import { SignInMethodEnum } from '../../../src/entities/user-sign-in-audit/enums/sign-in-method.enum.js'; | ||
| import { setSaasEnvVariable } from '../../utils/set-saas-env-variable.js'; | ||
|
|
||
| let app: INestApplication; | ||
| let currentTest: string; | ||
| let testUtils: TestUtils; | ||
|
|
||
| const mockFactory = new MockFactory(); |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variables mockFactory and testUtils are declared but never used in the test file. Consider removing them along with their imports (MockFactory and TestUtils) to keep the code clean. If they're intended for future use, add a comment explaining why they're kept.
| @Body() loginUserData: LoginUserDto, | ||
| ): Promise<ITokenExp> { | ||
| const { email, password, companyId: userCompanyId } = loginUserData; | ||
| const ipAddress = (request.headers['x-forwarded-for'] as string) || request.ip; |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The x-forwarded-for header can contain multiple IP addresses separated by commas when the request passes through multiple proxies (e.g., "client-ip, proxy1-ip, proxy2-ip"). Currently, the entire string is stored. Consider extracting only the first (client) IP address by splitting on comma: const ipAddress = ((request.headers['x-forwarded-for'] as string)?.split(',')[0]?.trim()) || request.ip;
| const ipAddress = (request.headers['x-forwarded-for'] as string) || request.ip; | |
| const ipAddress = ((request.headers['x-forwarded-for'] as string)?.split(',')[0]?.trim()) || request.ip; |
| let currentTest: string; | ||
| let testUtils: TestUtils; | ||
|
|
||
| const mockFactory = new MockFactory(); |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable mockFactory.
| test.serial(`${currentTest} should return sign-in audit logs with failed login attempt`, async (t) => { | ||
| try { | ||
| const adminUserRegisterInfo = await registerUserAndReturnUserInfo(app); | ||
| const { token, email, password } = adminUserRegisterInfo; |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable password.
| test.serial(`${currentTest} should return 400 for invalid status filter`, async (t) => { | ||
| try { | ||
| const adminUserRegisterInfo = await registerUserAndReturnUserInfo(app); | ||
| const { token, email } = adminUserRegisterInfo; |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable email.
| test.serial(`${currentTest} should return 400 for invalid sign-in method filter`, async (t) => { | ||
| try { | ||
| const adminUserRegisterInfo = await registerUserAndReturnUserInfo(app); | ||
| const { token, email } = adminUserRegisterInfo; |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable email.
| async (t) => { | ||
| try { | ||
| const adminUserRegisterInfo = await registerUserAndReturnUserInfo(app); | ||
| const { token, email } = adminUserRegisterInfo; |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable email.
|
@codex review |
No description provided.